DAOS-18431 bio: Set power management register on NVMe #17355
Conversation
Signed-off-by: Tom Nabarro <[email protected]>
|
Ticket title is 'NVMe power control feature' |
|
Test stage NLT on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-17355/1/testReport/ |
tanabarr
force-pushed
the
tanabarr/bio-powmanage-patch
branch
from
5e0062d to
0be43f7
Compare
Signed-off-by: Tom Nabarro <[email protected]>
tanabarr
force-pushed
the
tanabarr/bio-powmanage-patch
branch
from
0be43f7 to
8b31c49
Compare
kjacque
left a comment
kjacque
left a comment
There was a problem hiding this comment.
Overall looks reasonable. Just trying to understand the right way to interact with the spdk stuff.
If we're letting users configure this generally, it might be good to add an explicit parameter to the config file with more intuitively named values, based on whatever the 0-4 stand for. Not necessary in this PR, but something to think about.
| memset(&cmd, 0, sizeof(cmd)); | ||
| cmd.opc = SPDK_NVME_OPC_SET_FEATURES; | ||
| cmd.cdw10 = SPDK_NVME_FEAT_POWER_MANAGEMENT; | ||
| cmd.cdw11 = bio_spdk_power_mgmt_val; |
There was a problem hiding this comment.
union spdk_nvme_cmd_cdw11 has a member union spdk_nvme_feat_power_management feat_power_management that has multiple fields, not just the power state. So I think we should be using that to make sure we're setting the value correctly.
IMO we should also check the value taken from the env variable is in an acceptable range, preferably when we first ingest it. I had some trouble finding definitions for the power state values. Does SPDK define those somewhere?
There was a problem hiding this comment.
I think I wait for @Michael-Hennecke to verify the PR. this is more of an enablement change that will unblock some testing. Once verified we can look at tidying things.
There was a problem hiding this comment.
It seems like we at least need to check the value. I was wondering about what would happen if we set the env variable to some value larger than 5 bits, potentially overflowing into the other bit field ("workload hint"). Here's the struct definition I found: https://spdk.io/doc/unionspdk__nvme__feat__power__management.html
There was a problem hiding this comment.
@Michael-Hennecke thoughts on acceptable values?
Signed-off-by: Tom Nabarro <[email protected]>
| bb->bb_dev_health.bdh_io_channel = channel; | ||
|
|
||
| /* Set NVMe power management */ | ||
| rc = bio_set_power_mgmt(bb->bb_dev, channel); |
There was a problem hiding this comment.
I think we don't want this being skipped when health monitor is disabled? (see bypass_health_collect() at the beginning of this function).
| cmd.cdw10 = SPDK_NVME_FEAT_POWER_MANAGEMENT; | ||
| cmd.cdw11 = bio_spdk_power_mgmt_val; | ||
|
|
||
| rc = spdk_bdev_nvme_admin_passthru(d_bdev->bb_desc, channel, &cmd, NULL, 0, |
There was a problem hiding this comment.
This uses bdh_io_channel but doesn't hold reference count, that could cause issue when bio_dev_health being finalized before the completion. See bio_fini_health_monitoring().
Set NVMe power management values for SSDs by setting the new
engine DAOS_NVME_POWER_MGMT environment variable to an integer
normally (0-4). Value will be applied by SPDK on devices attached to
an engine process. The value will not be reset on engine exit.
Steps for the author:
After all prior steps are complete: